Skip to content

feat(brands): add explicit brand status-transition endpoint (LLMO-5587)#2621

Merged
aliciadriani merged 11 commits into
mainfrom
fix/LLMO-5587-brand-status-transition-endpoint
Jun 25, 2026
Merged

feat(brands): add explicit brand status-transition endpoint (LLMO-5587)#2621
aliciadriani merged 11 commits into
mainfrom
fix/LLMO-5587-brand-status-transition-endpoint

Conversation

@aliciadriani

@aliciadriani aliciadriani commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

Why

Active brands can be silently demoted to pending by the per-brand write primitives (updateBrand / upsertBrand) — no error to the editor, no alert. A demoted brand stops binding in the scheduler (the resolver requires status == active) and its site goes dark. This took express.adobe.com offline on 2026-06-05 (brand 40f16625, org 5d4e5082); same class as the MongoDB and Merck/MSD incidents documented in #2593. Incident: LLMO-5587.

What — PR1 of a staged rollout (additive)

Expand → migrate → contract. This is the additive expand step: a dedicated, intentful status-transition endpoint so legitimate demotions have a sanctioned path before the generic PATCH is locked down.

  • PATCH /v2/orgs/{spaceCatId}/brands/{brandId}/statustransitionBrandStatusForOrg (capability organization:write)
  • New setBrandStatus storage fn — minimal (status + updated_by only, no child-table sync), deliberately separate from updateBrand so it carries no demotion guard and the generic path can.
  • Body { "status": "active" | "pending" }; 400 invalid status / activate-without-site, 403/404/503 mirror the sibling brand handlers.
  • No DB migration. The row invariant (active ⇒ site_id) stays at the data layer via chk_active_brand_has_site_id.

Credit — lifted from #2504 (@igor-grubic)

The 23514 / chk_active_brand_has_site_id400 mapping is lifted verbatim from @igor-grubic's #2504 (closed in favor of this staged work). PR3 will additionally adapt #2504's needsExistingFetch existing-row fetch (broadened to select('status')) for the demotion comparison — credit carried forward there too. Pattern parallel: #2593 (@byteclimber) anchors immutability in the same brands-storage.js block.

Staged rollout / dependency chain

  1. PR1 (this PR, spacecat feat(brands): add explicit brand status-transition endpoint (LLMO-5587) #2621) — additive transition endpoint. Safe to merge/deploy independently.
  2. PR2 (elmo-ui feat: Add design documentation for the two-phase PLG tier lifecycle: PRE_ONBOARD (internal) and PLG (customer-visible). #2137) — migrate moveToPending (and approve) onto this endpoint. Must deploy before PR3.
  3. PR2 prod (elmo-ui chore: remove dead llmo-customer-config endpoint code #2139) — elmo-ui migration live in prod. Must be live before PR3 deploys.
  4. PR3 (spacecat feat(brands): guard against active->pending demotion in updateBrand/upsertBrand (LLMO-5587) #2637) — enable the 409 active→pending guard on the generic PATCH /brands/:brandId (+ upsertBrand by-name collision), re-land fix(brands): guard updateBrand against active-without-site_id (LLMO-5183) #2504's guards, add a BrandDemotionBlocked metric. Must NOT deploy before elmo-ui chore: remove dead llmo-customer-config endpoint code #2139 is live in prod, or it breaks elmo-ui's still-generic moveToPending with a loud 409.

Full deploy order: spacecat #2621 → elmo #2137 → elmo #2139 (prod) → spacecat #2637.

Related

Testing

  • setBrandStatus: 5 unit tests · transitionBrandStatusForOrg: 10 controller tests
  • Full suite green (11774 passing) · ESLint clean · docs:lint (OpenAPI) valid

🤖 Generated with Claude Code

PR1 of the staged active->pending demotion fix. Adds an additive, intentful
status-transition path so legitimate demotions have a sanctioned route before
the generic PATCH is locked down in PR3.

- PATCH /v2/orgs/:spaceCatId/brands/:brandId/status -> transitionBrandStatusForOrg
- setBrandStatus storage fn (minimal: status + updated_by, no child-table sync)
- 23514 chk_active_brand_has_site_id -> 400 mapping lifted from #2504 (Igor Grubic)
- OpenAPI spec + capability + tests (5 storage, 10 controller)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Jun 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions

Copy link
Copy Markdown

This PR will trigger a minor release when merged.

@aliciadriani aliciadriani left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — LLMO-5587 brand status-transition endpoint (#2621)

Summary: Adds an additive, intentful PATCH /v2/orgs/{spaceCatId}/brands/{brandId}/status endpoint backed by a minimal setBrandStatus (status + updated_by only, no child sync). This is the sanctioned demotion path the #2637 guard routes callers to. Clean and purely additive; full suite green. One real gap inline.

Sequencing

This PR should merge first#2637's 409 message references this endpoint as the demotion path. This PR is independently functional on its own.

Should Fix

  • deletedactive/pending reactivation isn't guarded or tested (inline on brands-storage.js / controller).

What's Good

  • Right design: a focused, intentful status setter kept separate from the guarded generic updateBrand, so the demotion guard can't be bypassed accidentally.
  • 23514 chk_active_brand_has_site_id → 400 mapping lifted from #2504 and credited to Igor.
  • Mirrors sibling brand handlers exactly (org/access/postgrest/resolve guards); OpenAPI + capability wired; good test coverage.


const { data, error } = await postgrestClient
.from('brands')
.update({ status, updated_by: updatedBy })

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should Fix: this update isn't scoped against soft-deleted brands. If resolveBrandUuid resolves a status='deleted' brand, this endpoint will silently reactivate it. Either intend that (document it) or block it — simplest is to add .neq('status', 'deleted') to the filter so a deleted brand matches no row and the controller returns 404. Add a test for the deleted-brand case.

Comment thread src/controllers/brands.js
if (!hasText(brandId)) {
return badRequest('Brand ID required');
}
if (status !== 'active' && status !== 'pending') {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: status is validated to {active, pending} but the brand's current status isn't checked — see the storage comment about not resurrecting deleted brands. Worth a test asserting a transition on a soft-deleted brand returns 404.

@aliciadriani aliciadriani self-assigned this Jun 19, 2026
… brand (PR #2621 review)

setBrandStatus now filters the update with .neq('status','deleted'), so a
soft-deleted brand matches no row and the transition endpoint returns 404
instead of silently reactivating it. Adds storage + controller tests.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@aliciadriani aliciadriani left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review fix applied — b55974a0

Addressed the Should-Fix (deleted-brand reactivation). Full suite green (11818 passing); ESLint clean.

  • setBrandStatus now filters the update with .neq('status','deleted'), so a soft-deleted brand matches no row and the endpoint returns 404 rather than silently reactivating it.
  • Added tests: storage (does not resurrect a soft-deleted brand → null) and controller (transitionBrandStatusForOrg returns 404 when the brand is soft-deleted).

Sequencing reminder: merge this PR before #2637 (its 409 message points here).

// Do not resurrect a soft-deleted brand via a status transition — a deleted
// brand matches no row here, so the caller gets a 404 (use a dedicated
// undelete flow if reactivation is ever needed).
.neq('status', 'deleted')

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Fixed (b55974a0): .neq('status','deleted') excludes soft-deleted brands from the status update — they match no row, so setBrandStatus returns null and the controller responds 404 (no accidental undelete). Covered by new storage + controller tests.

@aliciadriani aliciadriani removed the request for review from byteclimber June 19, 2026 10:54
@aliciadriani aliciadriani requested a review from MysticatBot June 19, 2026 11:40
@aliciadriani aliciadriani marked this pull request as ready for review June 19, 2026 14:25
…ForOrg (LLMO-5587)

codecov flagged 2 uncovered diff lines (brands.js: the !hasText(spaceCatId)
-> 'Organization ID required' branch). PR1 tested the invalid-UUID case but not
a missing org id. Add that test. Test-only; no source change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rainer-friederich rainer-friederich requested review from MysticatBot and removed request for MysticatBot June 24, 2026 06:34

@MysticatBot MysticatBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @aliciadriani,

Verdict: Approve - clean, focused additive endpoint that follows existing patterns exactly.
Complexity: HIGH - medium-to-large diff; new API surface (OpenAPI contract change).
Changes: Adds a dedicated PATCH /v2/orgs/{spaceCatId}/brands/{brandId}/status endpoint for intentful brand status transitions, with storage function, OpenAPI spec, and comprehensive tests (9 files).

Note: Recommend a human read before merge - this change modifies a shared API contract (docs/openapi/brands-v2-api.yaml). The bot review is a complement to, not a replacement for, a human read here.

Non-blocking (2): minor issues and suggestions
  • nit: OpenAPI description says "the generic PATCH refuses that transition" but that guard ships in PR3, not this PR - consumers reading the spec today may be confused by behavior that does not yet exist - docs/openapi/brands-v2-api.yaml:48
  • suggestion: An explicit controller-level test for the chk_active_brand_has_site_id 400 path (activate brand without site) would document the end-to-end contract; currently only tested at the storage layer - test/controllers/brands.test.js

Spec-divergence: no - PR implements the staged rollout described in the body; no referenced spec to diverge from.


Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 11m 4s | Cost: $6.85 | Commit: d497ace55211d62fb4361808b8de438946b325dd
If this code review was useful, please react with 👍. Otherwise, react with 👎.

@MysticatBot MysticatBot added ai-reviewed Reviewed by AI complexity:high AI-assessed PR complexity: HIGH needs-human-review AI reviewer recommends a human read before merge labels Jun 24, 2026
OpenAPI: soften forward-looking language — the generic PATCH demotion
guard ships in PR3, not here; change "refuses" (present tense) to "will
refuse" with an explicit (LLMO-5587 PR3) callout so consumers aren't
confused by behaviour that doesn't exist yet.

Test: add controller-level test for the chk_active_brand_has_site_id →
400 path (activate brand without base site). Documents the end-to-end
contract from HTTP surface to DB constraint mapping, complementing the
existing setBrandStatus storage-layer coverage.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@aliciadriani

Copy link
Copy Markdown
Collaborator Author

Addressed both nits from the MysticatBot review in `0f520a3`:

nit (OpenAPI forward-looking language): Changed "refuses that transition (LLMO-5587)" → "will refuse that transition" with an explicit "(LLMO-5587 PR3)" callout. Consumers reading the spec today see the description is forward-looking, not current behaviour.

suggestion (controller-level test for chk_active_brand_has_site_id): Added a test that wires a 23514 DB constraint error through the full controller path (transitionBrandStatusForOrgsetBrandStatus) and asserts a 400 response. Complements the existing storage-layer coverage and documents the end-to-end contract.

Alicia Adriani and others added 2 commits June 25, 2026 14:27
…LMO-5587)

Istanbul flagged lines 71-72 and 157 as uncovered branches — all existing
tests pass real objects to getDeliveryConfig/getHlxConfig, so the || {}
right-hand side never fires.

- getPreflightMissingConfigLabels: one test with null delivery and helix
  configs covers both fallbacks in a single call (lines 71-72).
- isPreflightSiteConfigReady: a stub returns a valid delivery config on
  the first call (so getPreflightMissingConfigLabels sees no missing
  labels) and null on the second call (line 157), exercising the fallback
  without early-returning from the missing-labels guard.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ilities (LLMO-5587)

facs-capabilities.js was introduced after this branch was cut; bring it
in now and add the new status-transition route under llmo/can_configure,
adjacent to the generic PATCH /v2/orgs/:spaceCatId/brands/:brandId entry.

Fixes the facs-capabilities invariant test: every route in routes/index.js
must appear in PRODUCTS_ROUTES or INTERNAL_ROUTES.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Keep the /status route entry added by this PR; main had no equivalent line.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@aliciadriani aliciadriani merged commit 7fbebb6 into main Jun 25, 2026
20 checks passed
@aliciadriani aliciadriani deleted the fix/LLMO-5587-brand-status-transition-endpoint branch June 25, 2026 14:02
solaris007 pushed a commit that referenced this pull request Jun 25, 2026
# [1.599.0](v1.598.1...v1.599.0) (2026-06-25)

### Features

* **brands:** add explicit brand status-transition endpoint (LLMO-5587) ([#2621](#2621)) ([7fbebb6](7fbebb6)), closes [#2504](#2504) [#2593](#2593) [#2137](#2137) [#2139](#2139) [#2637](#2637) [#2139](#2139) [#2137](#2137) [#2139](#2139) [#2504](#2504) [#2593](#2593) [#2300](#2300)
@solaris007

Copy link
Copy Markdown
Member

🎉 This PR is included in version 1.599.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

aliciadriani pushed a commit that referenced this pull request Jun 25, 2026
Merge main into branch to pick up #2621 (setBrandStatus endpoint).
Conflict in brands-storage.test.js resolved by keeping both describe blocks:
the demotion-guard tests (HEAD) and the setBrandStatus tests (#2621).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
aliciadriani added a commit that referenced this pull request Jun 25, 2026
…psertBrand (LLMO-5587) (#2637)

## Why
Active brands can be silently demoted to `pending` by the per-brand
write primitives (`updateBrand` / `upsertBrand`) — no error to the
editor, no alert. A demoted brand stops binding in the scheduler (the
resolver requires `status == active`) and its site goes dark. This took
**express.adobe.com** offline on 2026-06-05 (brand `40f16625`); same
class as the MongoDB and Merck/MSD incidents. Incident:
[LLMO-5587](https://jira.corp.adobe.com/browse/LLMO-5587).

## Deploy gate
This PR's 409 demotion guard must **NOT** deploy until elmo-ui #2139 is
live in prod. Otherwise elmo's still-generic "Move to pending" hits the
guard and 409s (loud, not silent).

**Order:** spacecat #2621 → elmo #2137 → elmo #2139 (prod) → this PR
(#2637).

## What
The generic per-brand write paths now **refuse** to silently demote an
active brand to pending; intentful demotions go through the dedicated
status-transition endpoint added in #2621.

- **`updateBrand`** — when a PATCH would take a persisted-`active` brand
to `pending` → **409** `brand_status_demotion_not_allowed`.
- **`upsertBrand`** — when a by-name (`org, name`) create/upsert would
resolve onto an existing **active** brand and demote it → same **409**.
(Closes the M1 collision vector.)
- The single existing-row fetch is reused for all three brand guards
(immutability, demotion, active-without-site).

## Credit — re-landed from #2504 (@igor-grubic)
#2504 was closed in favor of this staged work; its complementary guards
are re-landed here and credited in the code:
- the **`23514` / `chk_active_brand_has_site_id` → 400** mapping (covers
the SELECT→write race), and
- the **active-without-site promotion guard** (`status:'active'` with no
`site_id` → 400).
- Its **existing-fetch pattern** is the basis for the fetch here,
broadened from `site_id`-only to also read `status` for the demotion
comparison.

## Status-code semantics
- **409** — demotion of an active brand via the generic path (a
routing/state conflict; the caller should use the `/status` endpoint).
- **400** — row-invariant violations (activate-without-site, `23514`).

## Alerting — `BrandDemotionBlocked` metric
A best-effort CloudWatch **EMF** metric is emitted from the reject
branch (never affects the response), modeled on the LLMO-5150 P1
pattern:
- **Namespace:** `Mysticat/Brands` · **Metric:** `BrandDemotionBlocked`
(Count)
- **Dimensions:** `Environment`, `Operation` (`updateBrand` |
`createBrand`), `Product` (`x-product`)
- Each rejection also logs a **WARN** naming the caller (`org`, `brand`,
`updatedBy`) for attribution.
- **Alarm suggestion:** P1 on `sum(BrandDemotionBlocked) > 0` per
`Environment` (optionally split by `Operation`/`Product`); when it
fires, query the WARN logs to identify which caller still demotes via
the generic path.

## Sequencing
**#2621 (the `/status` transition endpoint) should merge first** — the
409 message references that endpoint as the sanctioned path for
intentful demotions. That said, **this PR is independently functional**:
the guard and metric work standalone; callers migrate to the `/status`
endpoint when the 409 + message tells them to (PR2 / elmo-ui
coordination was intentionally dropped).

## Related
- [LLMO-5587](https://jira.corp.adobe.com/browse/LLMO-5587) — incident
- #2621 — PR1, explicit `/status` transition endpoint (the sanctioned
demotion path)
- #2504 — closed; re-landed from (@igor-grubic)

## Testing
- Storage guard suite (demotion 409 ×2, promotion allowed,
no-false-positive on status-absent edit, promote-without-site 400,
`23514` ×2)
- Controller 409 cases for `updateBrandForOrg` + `createBrandForOrg`;
`metrics-emf` namespace test
- Full suite green; ESLint clean

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Alicia Adriani <aadriani+adobe@adobe.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Alicia Adriani <aadriani@adobe.com>
solaris007 pushed a commit that referenced this pull request Jun 25, 2026
# [1.600.0](v1.599.0...v1.600.0) (2026-06-25)

### Features

* **brands:** guard against active->pending demotion in updateBrand/upsertBrand (LLMO-5587) ([#2637](#2637)) ([410ae58](410ae58)), closes [#2139](#2139) [#2621](#2621) [#2137](#2137) [#2139](#2139) [#2504](#2504) [#2621](#2621) [#2621](#2621) [#2504](#2504)
ravverma added a commit that referenced this pull request Jun 25, 2026
…2691)

## ⚠️ Back office Outage mitigation — revert of #2686

Reverts the http-utils dependency bump from **#2686** (`96c5c104`),
downgrading `@adobe/spacecat-shared-http-utils` **1.31.0 → 1.30.0**.

**Why:** Back office production outage that began after the 1.31.0 bump
deployed. 1.31.0 (PR adobe/spacecat-shared#1712) shipped — alongside
`getFacsPermissions()` — the **IMS-handler RBAC block** (blocks IMS auth
for RBAC-enabled orgs), an auth-path change the back office relies on.
That is the most credible cause; detailed root cause to follow from
@ravverma.

**Scope:** only the http-utils version + its lockfile entry change
(verified: single `version` line, no unrelated dep drift). The three
feature PRs merged after #2686 (#2621, #2637, #2681) are **unaffected**.

**Trade-off:** this re-breaks the `GET /user/capabilities` JWT-layer
surfacing that #2686 fixed — but those endpoints are dev-gated
(`AWS_ENV=dev`) and not prod-facing, so restoring back-office
availability wins.

**Re-land options:**
- ship a patched http-utils that keeps `getFacsPermissions()` but gates
the IMS-handler RBAC block behind a verified flag, then re-bump.
- re-introduce the bump with the facsWrapper attachment once the
back-office auth interaction is fixed + tested.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
solaris007 pushed a commit that referenced this pull request Jun 25, 2026
## [1.601.2](v1.601.1...v1.601.2) (2026-06-25)

### Bug Fixes

* **deps:** redeploy http-utils 1.30.0 revert to prod (back office outage) ([#2692](#2692)) ([5c3cb11](5c3cb11)), closes [#2691](#2691)

### Reverts

* fix(deps): bump spacecat-shared-http-utils 1.30.0 to 1.31.0 ([#2691](#2691)) ([d8c8d8b](d8c8d8b)), closes [#2686](#2686) [adobe/spacecat-shared#1712](adobe/spacecat-shared#1712) [#2686](#2686) [#2621](#2621) [#2637](#2637) [#2681](#2681)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed Reviewed by AI complexity:high AI-assessed PR complexity: HIGH needs-human-review AI reviewer recommends a human read before merge released

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants